Add ShapePrimitive support for arcs and ellipses#8617
Add ShapePrimitive support for arcs and ellipses#8617VANSH3104 wants to merge 3 commits intoprocessing:dev-2.0from
Conversation
|
@ksen0, @davepagurek could you please review this and suggest any changes? |
davepagurek
left a comment
There was a problem hiding this comment.
Thanks for taking this on! I left a few comments about how to make these primitives work more like the others. We should also add some unit visual in both 2D and WebGL mode of these methods if we don't already have some to make sure things keep working.
|
|
||
| const primitive = new EllipsePrimitive(x, y, w, h); | ||
| const shape = { accept(visitor) { primitive.accept(visitor); } }; | ||
| this.drawShape(shape); |
There was a problem hiding this comment.
It looks like drawShape already handles clipping so we can remove the clipping code above:
p5.js/src/core/p5.Renderer2D.js
Lines 269 to 279 in 01aa360
src/core/p5.Renderer2D.js
Outdated
| } | ||
|
|
||
| const primitive = new EllipsePrimitive(x, y, w, h); | ||
| const shape = { accept(visitor) { primitive.accept(visitor); } }; |
There was a problem hiding this comment.
Rather than making an object that isn't a full p5.Shape, could we instead make a new real p5.Shape and create a method on it that adds an ellipse primitive? similar to the relation between these methods and their corresponding primitives:
p5.js/src/shape/custom_shapes.js
Lines 891 to 906 in 618e4c8
src/shape/custom_shapes.js
Outdated
| const centerY = arc.y + arc.h / 2; | ||
| const radiusX = arc.w / 2; | ||
| const radiusY = arc.h / 2; | ||
| const numPoints = Math.max(3, this.curveDetail); |
There was a problem hiding this comment.
curveDetail is a multiplier on the length, so numPoints should be something like ceil(this.curveDetail * arcLength)
src/shape/custom_shapes.js
Outdated
| #stop; | ||
| #mode; | ||
| // vertexCapacity 0 means this primitive should not accumulate normal path vertices | ||
| #vertexCapacity = 0; |
There was a problem hiding this comment.
Rather than a capacity of zero, possibly we can contain a vertex representing the start and the end so that we can include all the other properties on a general vertex, which might include texture coordinates, fill, and stroke colors in webgl? Ideally it should go through #generalVertex in Shape to get all of that functionality.
src/shape/custom_shapes.js
Outdated
|
|
||
| for (let i = 0; i <= numPoints; i++) { | ||
| const angle = arc.start + (arc.stop - arc.start) * (i / numPoints); | ||
| verts.push(new Vertex({ |
There was a problem hiding this comment.
Similar to what I mentioned above in ArcPrimitive, ideally we should have some vertices in the primitive already so we can copy their data all around the ellipse but update the position part (the first 2 elements) for arcs and ellipses. For arcs, ideally we would also interpolate the properties of the vertex from the start to the end.
There was a problem hiding this comment.
Can you explain it more please?
There was a problem hiding this comment.
For our current vertices in beginShape/endShape, they have methods on the Shape class to add them that go through #generalVertex:
p5.js/src/shape/custom_shapes.js
Lines 891 to 913 in 209f8d1
That's generally to support a construct in beginShape/endShape where you can call something in the form of somethingVertex(x, y, [z], [u], [v]), letting you specify texture coordinates between points, and call fill() or stroke() between vertices too. There's a proposal #6459 on what arcs would look like in a per-vertex form like that, and arcTo in the canvas 2D API works like that too.
We don't need to actually implement arcVertex yet, but ideally we'd start to set ourselves up to be able to do that eventually. That likely means having an arc segment with #vertexCapacity of 2, so you can put a vertex at the start and a vertex at the end, referring to these:

There was a problem hiding this comment.
Arguably the arcTo primitive could be separate from the arc primitive, but since a lot will be shared, I think the idea would be ideally to build it in a way where arc is made out of arcTo-style primitives. But there's a case to be made that we only get that benefit once we have arcTo, and having any support for arc unblocks other features like SVG export. What do you think?
|
@davepagurek thanks for the thorough review! I've made updates based on your recommendations.Could you please review and let me know if the changes correctly incorporate your feedback? Thanks! |
Resolves #8277
Changes:
PR Checklist
npm run lintpasses